Address bug with CSV import on sync job #314
Conversation
msheiny
force-pushed
the
msheiny_job_fix
branch
from
0e7a933 to
7c8a0ca
Compare
msheiny
force-pushed
the
msheiny_job_fix
branch
from
7c8a0ca to
4c27c26
Compare
msheiny
requested review from
glennmatthews,
chadell,
susanhooks,
Dav-C,
jeffkala and
scetron
as code owners
| @@ -118,7 +118,7 @@ def netmiko_send_commands( | |||
| return Result( | |||
| host=task.host, result=f"{task.host.name} has missing definitions in command_mapper YAML file.", failed=True | |||
| ) | |||
| if orig_job_kwargs["connectivity_test"]: | |||
| if orig_job_kwargs.get("connectivity_test", False): | |||
There was a problem hiding this comment.
connectivity is guaranteed to be present right? Nit pick but just curious on the .get() instead of key call.
There was a problem hiding this comment.
Well, it's not "guaranteed" to be present because its all getting passed in via optional kwargs from method to method. This is one of the dangers of passing around **kwargs that are optional to python but required to the logic.
one thing I could tweak is to make it a required field on the method and make the subsequent changes on upstream callers callers in this PR. OR i could add a check at the top of this method to throw an exception if its not passed.
There was a problem hiding this comment.
I added the .get as a safety measure in case someone used this method in the future and didn't pass it BUT yeah i think its better to explicitly throw an exception OR require it as a method variable
Closes: #313
What's Changed
connectivity_testis passed through under theSync from Networkjob when using a CSVI added some additional tests, made the default behavior more resilient if the argument isn't passed for some reason, and tested the change explicitly on my local box after the code changes (using a
containerlabdevice):To Do